Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ContextualSaveBar] Add support for custom saveAction #10249

Closed
wants to merge 1 commit into from

Conversation

chloerice
Copy link
Member

@chloerice chloerice commented Aug 25, 2023

Note

This PR should not be shipped until the context bar redesign ships and we know whether or not this is still necessary to support.

WHY are these changes introduced?

Fixes https://github.com/Shopify/polaris-internal/issues/1376

ContextualSaveBar doesn't accept a ReactNode for saveAction right now, so the improved API for split buttons shipped in #10182 can't be implemented in it.

WHAT is this pull request doing?

This PR:

  • Adds support for setting JSX on the saveAction prop
  • Removes unneeded overrides of Button color properties to invert the ContextualSaveBar saveAction
  • Updates the "With custom primary action" of Page example to a realistic admin use case (the split button)
Screenshot 2023-08-25 at 6 03 25 PM

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

@chloerice chloerice added the #gsd:36573 Polaris API Alignment label Aug 25, 2023
@chloerice chloerice changed the base branch from main to next August 25, 2023 21:14
Comment on lines 796 to 519
&:active svg,
&.pressed svg {
fill: var(--p-color-icon-critical-strong-active-experimental);
}
&:hover {
// stylelint-disable-next-line -- Button custom properties
--pc-button-icon-color: var(
--p-color-icon-critical-strong-hover-experimental
);
}

&.disabled svg {
fill: var(--p-color-icon-disabled);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two declarations were duplicates once the unneeded svg tag is removed from it, so the custom property is set at the top of the equivalent class' declarations on 606 and 664.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore)

Comment on lines 605 to 608
// stylelint-disable-next-line -- Button custom properties
--pc-button-icon-color: var(
--p-color-icon-critical-strong-active-experimental
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore)

Comment on lines 663 to 664
// stylelint-disable-next-line -- Button custom properties
--pc-button-icon-color: var(--p-color-icon-disabled);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore)

@@ -40,7 +40,7 @@ export interface ContextualSaveBarProps {
/** Accepts a string of content that will be rendered to the left of the actions */
message?: string;
/** Save or commit contextual save bar action with text defaulting to 'Save' */
saveAction?: ContextualSaveBarAction;
saveAction?: ContextualSaveBarAction | React.JSX.Element;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ReactNode is too wide of a net so this is more specific, as we only want to accept JSX elements not strings etc)

@chloerice chloerice force-pushed the csb-custom-save-action branch 4 times, most recently from 060c785 to 3b3f0d0 Compare August 25, 2023 21:54
Comment on lines 7 to 9
- [Migrating from v11 to v12](#migrating-from-v11-to-v12)
- [Table of Contents](#table-of-contents)
- [Quick migration guide](#quick-migration-guide)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens automatically no matter what extension I disable 😤

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore)

Comment on lines 163 to 166
svg {
// stylelint-disable-next-line -- generated by polaris-migrator DO NOT COPY
fill: var(--pc-button-icon-color);
}
Copy link
Member Author

@chloerice chloerice Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this being declared over and over again, it's now declared once and the custom property is set in the class and interaction state declarations instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ignore)

@chloerice chloerice added 🤖Skip Major Check next Track all things related to next major release 🤖Skip Comment Check Skip the migrator comment CI check labels Aug 25, 2023
@chloerice chloerice self-assigned this Sep 19, 2023
Base automatically changed from next to main October 9, 2023 03:02
@alex-page alex-page added #gsd:37811 and removed #gsd:36573 Polaris API Alignment labels Dec 4, 2023
@lgriffee
Copy link
Member

@chloerice Is this still being worked on? Or is this work now a part of the overlay patterns project?

@lgriffee lgriffee added #gsd:38846 Admin Quality Improvements (Q1 2024) Priority: Low Small, subtle details labels Jan 12, 2024
@chloerice
Copy link
Member Author

Yes, I'll jump back into this Monday! I'd held off on it because I was updating the way we handle SVG fill in Button (nerd-sniped 😅) and wanted to split that out. Sam did this in the Button refactor, so I can just clean this branch up to only the CSB changes 🚀

@lgriffee
Copy link
Member

@chloerice Awesome, thanks so much Chloe! Also just wanted to let you know I created an accompanying issue for this PR since I got a request to remove any PRs from our project board. Feel free to edit the issue if needed!

@chloerice chloerice marked this pull request as ready for review February 26, 2024 23:03
@chloerice chloerice removed 🤖Skip Comment Check Skip the migrator comment CI check 🤖Skip Major Check next Track all things related to next major release labels Feb 26, 2024
@sophschneider
Copy link
Contributor

sophschneider commented Feb 27, 2024

is it possible to hold off on this until we redesign the CSB? it might be harder for us to move/override the CSB save if we don't know for sure that it is a button.

We're also introducing a new button style so not sure how it would work for the connected disclosure. Is this needed somewhere in the admin right now?

@chloerice
Copy link
Member Author

chloerice commented Feb 29, 2024

is it possible to hold off on this until we redesign the CSB? it might be harder for us to move/override the CSB save if we don't know for sure that it is a button.

Yes, this totally makes sense to hold off on 💯 Moved to Icebox and noted in description!

We're also introducing a new button style so not sure how it would work for the connected disclosure. Is this needed somewhere in the admin right now?

@sophschneider No, not that I'm aware of 👀 It was just a regression of support when we removed the connectedDisclosure prop so I put a PR so I wouldn't forget as we were doing v12 migrations 😅

@chloerice chloerice marked this pull request as draft February 29, 2024 14:49
@chloerice chloerice removed the request for review from sophschneider February 29, 2024 14:49
@chloerice chloerice removed the #gsd:38846 Admin Quality Improvements (Q1 2024) label Feb 29, 2024
@github-actions github-actions bot added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 19, 2024
@chloerice chloerice closed this Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. Priority: Low Small, subtle details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants